Draft
Conversation
Restructure modX::_log() to dispatch to a PSR-3 logger when one is registered via the new setLogger() method. The PSR-3 logger replaces legacy FILE/ECHO/HTML output while modRegister logging is preserved. Includes 20 tests: 11 baseline tests locking down existing legacy logging behavior, and 9 tests covering PSR-3 integration.
Member
|
Thanks for working on this, @pbowyer! I'm working out a release plan for a new minor of xPDO. Let's keep this in draft until we have that available. |
Contributor
Author
|
Since posting this I think I have swung in favour of 1B... 😊 |
5c8bbc4 to
41e6dea
Compare
41e6dea to
fa6c174
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Previously MODX hasn't supported PSR-3 logging. This was added to xPDO (commit modxcms/xpdo@55fd343) but
modX::_log()overrides it in several code paths, so a logger registered on xPDO never sees all of MODX's log output.This PR adds
setLogger(LoggerInterface $logger)to MODX, restructures_log()to dispatch to a PSR-3 logger when one is set, and adds 20 new tests (11 baseline, 9 PSR-3).Backward compatibility
When no PSR-3 logger is registered (
$this->logger === null, the default), all logging behaviour is identical to the previous implementation. I added 11 tests as a baseline to verify this.Design decisions and alternatives
Three questions came up during implementation where the answer isn't obvious. I went back and forth on them @opengeek and picked one to finish the code, but without a strong view on them.
1. Should the PSR-3 logger respect MODX's
logLevel, or receive everything?A. Respect
logLevel(chosen): The PSR-3 logger only sees messages that pass thelogLevelthreshold. Users call$modx->setLogLevel(modX::LOG_LEVEL_DEBUG)when they want full visibility, and can filter further in their logger config (e.g. Monolog handler levels).B. Send everything to the logger: The PSR-3 logger receives all messages regardless of
logLevel. The logger's own filtering decides what to keep.I chose A because it seemed right, but have been doubting that since. Initially I chose 2B (below) so it made sense for performance, as every
log()call that isn't filtered out early triggersdebug_backtrace()in the legacy path to resolve the calling file and line. The earlylogLevelcheck (an integer comparison) skips all that work for messages below threshold. With option B and the defaultLOG_LEVEL_ERROR, every DEBUG/INFO/WARN call would still build context and dispatch to the logger. The cost of A is one extra line of config (setLogLevel) when setting up a logger.But then I switched to 2A so the performance impact of 1B would be minimal.
To switch to B: Remove the early return at the top of
_log()for the PSR-3 path.2. Should
debug_backtrace()be called for the PSR-3 path?A. Skip backtrace for PSR-3 (chosen): The
fileandlinecontext fields are only populated if the caller passed them (e.g.__FILE__,__LINE__). When not provided, they're empty strings. Monolog users addIntrospectionProcessorfor automatic caller info.B. Always resolve backtrace: Every log call resolves the backtrace so that
file/lineare always in the PSR-3 context, matching legacy behavior.I chose A because
debug_backtrace()is the most expensive operation in the log path. I felt this should be left to the developer as e.g. Monolog'sIntrospectionProcessordoes the same thing but only for messages that survive handler-level filtering. So a DEBUG message discarded by the handler never triggers a backtrace.To switch to B: Move the backtrace resolution to before the
_dispatchToPsrLogger()call, so the PSR-3 context always has file/line. Simpler for users who don't want to configure logging processors, but costs a backtrace on every dispatched log call.3. Should the PSR-3 logger replace legacy output, or be additive?
A. Replace legacy output (chosen): When a PSR-3 logger is set, legacy FILE/ECHO/HTML/ARRAY output is suppressed. Only modRegister logging is preserved alongside the PSR-3 logger.
B. Additive (both): Both the PSR-3 logger and legacy targets receive messages. The existing
error.logcontinues alongside the custom logger.C. Configurable: Add an option (e.g.
psr_logger_exclusive) to let users choose at runtime.I chose A because if you've set up a PSR-3 logger, you're taking control of where logs go. Writing to
error.logat the same time is confusing and wasteful. modRegister is preserved because it's a message queue, not traditional logging.To switch to B: In the restructured
_log(), remove the early return after the PSR-3 dispatch and let execution fall through to the legacy path. The$this->logger = null/try/finallypattern already exists for the legacyparent::_log()call and would prevent double-dispatch.To switch to C: Add a config option check before the early return, e.g.:
Other notes
xPDOLogger exclusion: The bundled
xPDOLogger(standalone xPDO's default logger) is excluded from MODX's PSR-3 dispatch. ItshandleXpdo()callsexit()for FATAL, while MODX usessendError('fatal')which renders a proper error page. AllowingxPDOLoggerwould change the current MODX behaviour, so I've assumed it is for standalone xPDO use only.FATAL handling: MODX uses
sendError('fatal')for fatal log messages, notexit(). The PSR-3 logger receives the FATAL message (mapped toLogLevel::CRITICAL) beforesendError()runs.Backward compatibility: When no PSR-3 logger is set (
$this->logger === null, the default), behavior is identical to the previous implementation. The baseline tests verify this.Related issue(s)/PR(s)